feat: upload input v3 image preview improvement#263
Conversation
📝 WalkthroughWalkthroughThis PR refactors file upload thumbnail previews by creating blob URLs directly in UploadInputV3 for newly added image files, optimizing ProgressiveImg to handle local URLs synchronously, and managing the preview lifecycle through proper revocation on cancel/error and transfer to state on completion. ChangesBlob URL Preview Caching and Lifecycle
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Around line 182-194: The current preview assignment uses FIFO shift on
pendingPreviewsRef.current and assumes server-returned value order matches
upload order, causing mis-matches; update the logic to match previews to files
by a stable identifier instead of insertion order: ensure preview entries in
pendingPreviewsRef include the original client filename (e.g.,
preview.originalName or preview.clientFilename) at enqueue time, build a map
from that original name to dataURL, then iterate newFiles (from value) and for
each file try to match by comparing f.filename or f.originalName to the preview
map key (and use a heuristic like contains/endsWith if the server adds
prefixes); assign matched previews into updates and only fall back to shift()
for any remaining unmatched files; update the code paths around
pendingPreviewsRef, newFiles, and setFilePreviews to use this name-based
matching.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7d9718e3-5485-4d80-a2ed-9b8ff5300c12
📒 Files selected for processing (3)
src/components/inputs/upload-input-v3/dropzone-v3.jssrc/components/inputs/upload-input-v3/index.jssrc/components/progressive-img/index.js
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
priscila-moneo
force-pushed
the
feature/upload-input-v3-image-preview-improvement
branch
from
4610604 to
32e967f
Compare
| * @constructor | ||
| */ | ||
| const ProgressiveImg = ({ placeholderSrc, src, ...props }) => { | ||
| const isCancelled = useRef(false); |
There was a problem hiding this comment.
are you sure to do this ? is not the same to use a Ref or a let
There was a problem hiding this comment.
Yes, I know. This change is intentional. The useRef is shared across all effect runs, so when src changes, the cleanup sets isCancelled.current = true on the shared ref — and the new effect's onload sees true and bails out immediately, leaving the image stuck in the blurred placeholder state forever. The let cancelled closure variable is scoped to each individual effect run, so each one starts fresh with false.
| const handleThumbnail = useCallback((file, dataURL) => { | ||
| pendingPreviewsRef.current.push({ name: file.name, size: file.size, dataURL }); | ||
| setUploadingFiles(prev => prev.map(f => | ||
| f.name === file.name && f.size === file.size ? { ...f, previewUrl: dataURL } : f |
There was a problem hiding this comment.
what happens if changing for another file with same name and same size ?
There was a problem hiding this comment.
Fair point — while it's an edge case (same name + same size is essentially the same file), I've replaced dataURL from the thumbnail event with URL.createObjectURL(file) in addedfile. Each File object gets a unique blob URL by reference, so collisions are impossible regardless of name or size. As a bonus, the preview is now available immediately when the file is selected instead of waiting for Dropzone to generate the thumbnail. Blob URLs are revoked on cancel/error to avoid memory leaks.
| const [errorFiles, setErrorFiles] = useState([]); | ||
| const [filePreviews, setFilePreviews] = useState({}); | ||
| const prevValueRef = useRef(value); | ||
| const pendingPreviewsRef = useRef([]); |
There was a problem hiding this comment.
seems like filePreviews already has this information, why you need this ref ?
There was a problem hiding this comment.
Simplification: collapse the preview state into a single store
The current design stores each dataURL in two places simultaneously — uploadingFiles[i].previewUrl (for the in-progress row) and pendingPreviewsRef (so useLayoutEffect can re-key it by
server filename later). This forces three separate cleanup sites that must all stay in sync: handleFileError, handleDeleteUploading, and useLayoutEffect.
All of this goes away if handleThumbnail stores directly into filePreviews keyed by the original filename:
const handleThumbnail = useCallback((file, dataURL) => {
setFilePreviews(prev => ({ ...prev, [file.name]: dataURL }));
}, []);
Then:
- Uploading rows read filePreviews[file.name] at render time — previewUrl can be dropped from uploadingFiles and handleAddedFile no longer needs to set it.
- Committed value rows already fall back to serverPreviewSrc, so the stem-match can be done inline at read time: filePreviews[filename] || filePreviews[Object.keys(filePreviews).find(k =>
filename.includes(k.replace(/.[^.]+$/, '')))] || serverPreviewSrc. - pendingPreviewsRef, prevValueRef, and the entire batched matching algorithm in useLayoutEffect are removed.
- Cleanup on cancel/error becomes a single setFilePreviews delete instead of a coordinated mutation across two collections.
| // Once the parent updates value, remove all completed files from uploadingFiles | ||
| useEffect(() => { | ||
| // Once the parent updates value, remove completed uploading files and assign local previews to new files | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
I don't see the use case for this change, can you explain ?
There was a problem hiding this comment.
useLayoutEffect runs synchronously before the browser paints, so when value updates, the completed uploading row is removed before the user sees anything. With useEffect it runs after paint, causing a one-frame flash where the file appears twice — once as "Complete" in the uploading section and once in the committed value section.
There was a problem hiding this comment.
this just hides the symptom but does not address the re-render . which is caused by this
There was a problem hiding this comment.
Done the refactor you mention and prevValueRef, pendingPreviewsRef and the batched matching are all gone. But useLayoutEffect is still needed though: when value updates, both the completed uploading row and the new value row land in the same render, and without it the user sees them both for one frame before the cleanup runs. It's not hiding a symptom, it's preventing a real visual duplicate.
There was a problem hiding this comment.
I'm working on the last of your comments and will update this PR with the latest code, will tag you so you can review it.
| const entry = | ||
| avail(e => e.name === f.filename) ?? | ||
| avail(e => { const s = e.name.replace(/\.[^.]+$/, ''); return s && f.filename.includes(s); }) ?? | ||
| avail(() => true); |
There was a problem hiding this comment.
on multiple uploads at same time, this can potentially match the wrong file. Worth a comment at least
Signed-off-by: Priscila Moneo <priscila_moneo@hotmail.com.ar>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/inputs/upload-input-v3/index.js (1)
210-214:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreview mapping is still ambiguous for same-size concurrent uploads.
At Line 212, matching uses only
response.size, so two different files with identical sizes can receive swapped previews. Please map with a stable per-file identifier (client upload id echoed by server, or an explicit correlation token) instead of size-only matching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/inputs/upload-input-v3/index.js` around lines 210 - 214, The current preview-assignment logic in the setUploadingFiles callback uses response.size to match upload entries (response.name/response.size and entry.previewUrl), which causes wrong previews for concurrent same-size files; change the protocol to use a stable per-file identifier (e.g. response.uploadId or response.correlationId) returned by the server and store that id on the client-side upload entries, then in setUploadingFiles find the entry by that uploadId instead of size and call setFilePreviews with the stable key (e.g. setFilePreviews(p => ({ ...p, [response.uploadId]: entry.previewUrl }))). Update any code that creates upload entries to include the client upload id so matching is deterministic.
🧹 Nitpick comments (2)
src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js (2)
350-358: ⚡ Quick winPreserve and restore original
URLmethods instead of deleting globals.At Line 356–Line 357, deleting global methods can pollute other tests. Store originals and restore them in
afterEachfor isolation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js` around lines 350 - 358, The tests are deleting global URL methods which can leak into other tests; modify the beforeEach/afterEach so beforeEach saves the originals (e.g., const originalCreateObjectURL = URL.createObjectURL; const originalRevokeObjectURL = URL.revokeObjectURL) then mocks with jest.fn in beforeEach, and in afterEach restore them (URL.createObjectURL = originalCreateObjectURL; URL.revokeObjectURL = originalRevokeObjectURL) instead of using delete; update the beforeEach/afterEach in upload-input-v3.test.js around the existing beforeEach/afterEach blocks referencing URL.createObjectURL and URL.revokeObjectURL.
414-434: ⚡ Quick winAdd a collision test for parallel uploads with identical file sizes.
Current coverage validates out-of-order responses but only with unique sizes. Add a case where two image uploads have the same
sizeto catch preview mis-assignment regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js` around lines 414 - 434, Add a new unit test alongside the existing "correctly maps previews for parallel uploads using response size" that reproduces a collision where two uploads have identical size: use the same pattern of calling dropzoneCallbacks.onAddedFile for two distinct filenames (e.g., 'sunset.jpg' and 'portrait.jpg') with identical size values, call dropzoneCallbacks.onFileCompleted for both, then simulate server responses in reverse order via dropzoneCallbacks.onUploadComplete with server filenames (e.g., '246_portrait_abc123.jpg' and '246_sunset_def456.jpg'), rerender UploadInputV3 with value containing those server filenames and sizes, and assert that screen.getByRole('img', { name: ... }) returns the expected blob src mapping to the original preview names (e.g., 'blob:portrait.jpg' and 'blob:sunset.jpg') to catch preview mis-assignment when sizes collide.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Line 70: Add a useEffect that revokes cached blob URLs on unmount and whenever
filePreviews changes: implement useEffect(() => { return () => {
Object.values(filePreviews).forEach(url => { if (url) URL.revokeObjectURL(url)
}) } }, [filePreviews]); this ensures any object URLs created earlier (look for
places that call URL.createObjectURL in the upload preview code around the
filePreviews setter) are revoked; also ensure any code path that replaces an
individual preview calls URL.revokeObjectURL on the old URL before overwriting
filePreviews via setFilePreviews.
---
Duplicate comments:
In `@src/components/inputs/upload-input-v3/index.js`:
- Around line 210-214: The current preview-assignment logic in the
setUploadingFiles callback uses response.size to match upload entries
(response.name/response.size and entry.previewUrl), which causes wrong previews
for concurrent same-size files; change the protocol to use a stable per-file
identifier (e.g. response.uploadId or response.correlationId) returned by the
server and store that id on the client-side upload entries, then in
setUploadingFiles find the entry by that uploadId instead of size and call
setFilePreviews with the stable key (e.g. setFilePreviews(p => ({ ...p,
[response.uploadId]: entry.previewUrl }))). Update any code that creates upload
entries to include the client upload id so matching is deterministic.
---
Nitpick comments:
In `@src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.js`:
- Around line 350-358: The tests are deleting global URL methods which can leak
into other tests; modify the beforeEach/afterEach so beforeEach saves the
originals (e.g., const originalCreateObjectURL = URL.createObjectURL; const
originalRevokeObjectURL = URL.revokeObjectURL) then mocks with jest.fn in
beforeEach, and in afterEach restore them (URL.createObjectURL =
originalCreateObjectURL; URL.revokeObjectURL = originalRevokeObjectURL) instead
of using delete; update the beforeEach/afterEach in upload-input-v3.test.js
around the existing beforeEach/afterEach blocks referencing URL.createObjectURL
and URL.revokeObjectURL.
- Around line 414-434: Add a new unit test alongside the existing "correctly
maps previews for parallel uploads using response size" that reproduces a
collision where two uploads have identical size: use the same pattern of calling
dropzoneCallbacks.onAddedFile for two distinct filenames (e.g., 'sunset.jpg' and
'portrait.jpg') with identical size values, call
dropzoneCallbacks.onFileCompleted for both, then simulate server responses in
reverse order via dropzoneCallbacks.onUploadComplete with server filenames
(e.g., '246_portrait_abc123.jpg' and '246_sunset_def456.jpg'), rerender
UploadInputV3 with value containing those server filenames and sizes, and assert
that screen.getByRole('img', { name: ... }) returns the expected blob src
mapping to the original preview names (e.g., 'blob:portrait.jpg' and
'blob:sunset.jpg') to catch preview mis-assignment when sizes collide.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 060f9ed7-3bec-49bb-a60b-9791fd9adcfc
📒 Files selected for processing (4)
src/components/inputs/upload-input-v3/__tests__/upload-input-v3.test.jssrc/components/inputs/upload-input-v3/index.jssrc/components/progressive-img/__tests__/progressive-img.test.jssrc/components/progressive-img/index.js
✅ Files skipped from review due to trivial changes (1)
- src/components/progressive-img/tests/progressive-img.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/progressive-img/index.js
| const dropzoneInstanceRef = useRef(null); | ||
| const [uploadingFiles, setUploadingFiles] = useState([]); | ||
| const [errorFiles, setErrorFiles] = useState([]); | ||
| const [filePreviews, setFilePreviews] = useState({}); |
There was a problem hiding this comment.
Add lifecycle cleanup for cached blob URLs.
filePreviews entries are never revoked on component unmount, so object URLs can leak memory beyond the upload flow. Add a cleanup effect that revokes all remaining blob URLs.
Suggested patch
+ React.useEffect(() => {
+ return () => {
+ Object.values(filePreviews).forEach((url) => {
+ if (url) URL.revokeObjectURL(url);
+ });
+ };
+ }, [filePreviews]);Also applies to: 169-172
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/inputs/upload-input-v3/index.js` at line 70, Add a useEffect
that revokes cached blob URLs on unmount and whenever filePreviews changes:
implement useEffect(() => { return () => {
Object.values(filePreviews).forEach(url => { if (url) URL.revokeObjectURL(url)
}) } }, [filePreviews]); this ensures any object URLs created earlier (look for
places that call URL.createObjectURL in the upload preview code around the
filePreviews setter) are revoked; also ensure any code path that replaces an
individual preview calls URL.revokeObjectURL on the old URL before overwriting
filePreviews via setFilePreviews.
santipalenque
left a comment
santipalenque
left a comment
There was a problem hiding this comment.
LGTM, thank you @priscila-moneo
2 participants
ref: https://app.clickup.com/t/86ba8wmgh
Summary by CodeRabbit
New Features
Improvements
Tests